rdrand: Add 32-bit x86 support #134
Merged
+35
−11
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
josephlr
force-pushed
the
x32
branch
3 times, most recently
from
a97b5bd to
0b6d683
Compare
Also add tests for 32-bit x86
newpavlov
approved these changes
Feb 20, 2020
dhardy
approved these changes
Feb 20, 2020
Ya, it would be nice if we had a way to detect CPUID support based on the build target, but that doesn't work consistently across arches. Something like Given that only suuuuper old targets don't have CPUID, I think it's fine if they get a UD exception when running this crate. I'll be shocked if this ever happens. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The code in
rdrand.rsis mostly straightforward. We just switch between usingcore::arch::x86_64::_rdrand64_stepandcore::arch::x86::_rdrand32_step. By havingWORD_SIZEdiffer betweenx86_64andx86, most of the code can remain the same while not losing any efficiency (as would be the case if we used_rdrand32_stepon both platforms).Similar to what we were already doing on
x86_64, we now always test the RDRAND implementation on allx86targets. I also improved our testing coverage, we now run tests on the following targets:x86_64-unknown-linux-gnux86_64-unknown-linux-musli686-unknown-linux-gnui686-unknown-linux-muslThis is possible as 64-bit Linux supports running 32-bit binaries.
Remaining questions
Right now we just assume that we will always have CPUID. This is true for all currently supported Rust x86 targets (as CPUID has been around for ~27 years). However, in an ideal world, we would use
has_cpuidbefore invoking__cpuid, but that function isn't stable (and will likely never be stable).Is it fine to just assume CPUID is always supported (on non-SGX targets)? Or should we do something else to handle the case of CPUID not existing.